-
Notifications
You must be signed in to change notification settings - Fork 2
Added physics types. #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Gravity, Velocity, Position, DrawColor, more for individual points. Working on a unified PhysicsObject class to treat many points as one object.
| return val; | ||
| } | ||
| public static float ClampMin(float min, float val) | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per discussion with Peter, the idea is a one-side capping of the value, rather than automatic return of a max value, as Math.Max would do.
| } | ||
| return val; | ||
| } | ||
| public static float ClampMax(float max, float val) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per discussion with Peter, the idea is a one-side capping of the value, rather than automatic return of a min value, as Math.Min would do.
glen3b
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked too deeply at the actual physics implementation, but I've briefly reviewed the general API and I have a handful of preliminary pointers.
| { | ||
| public static class PhysicsExtensions | ||
| { | ||
| public static bool ContainsPoint(this List<PhysicsPoint> points, Point point, bool mustInteractWithEnvironment = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it could take an IEnumerable<PhysicsPoint> instead of a list, is there any particular reason you need it to be ordered?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, IEnumerable is a better choice here.
| return false; | ||
| } | ||
|
|
||
| public static Point BottomLeft(this List<PhysicsPoint> points) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider whether this can take an IEnumerable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, IEnumerable is a better choice here.
|
|
||
| namespace ConsoleGameLib | ||
| { | ||
| public class PhysicsPoint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider overloading Equals, GetHashCode, and the corresponding operators.
|
|
||
|
|
||
|
|
||
| public List<PhysicsPoint> Contents |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Evaluate other possible collection types.
|
|
||
| Microsoft Visual Studio Solution File, Format Version 11.00 | ||
| # Visual C# Express 2010 | ||
| Microsoft Visual Studio Solution File, Format Version 12.00 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break compatibility with developing in VS express 2010, this is probably OK but is worth evaluating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok with that. VS2010 is a discontinued product, it can't even be installed (or more precisely, activated once installed) anymore.
| { | ||
|
|
||
| static void Main(string[] args) | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please comment your example code extensively so it is easy to tell what it does without referring to other code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments are good. I agree with this suggestion
| { | ||
| foreach(PhysicsPoint entry in points) | ||
| { | ||
| if(entry.Position.X == point.X && entry.Position.Y == point.Y && (mustInteractWithEnvironment == true ? entry.InteractsWithEnvironment : true)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
entry.Position is of type ConsoleGameLib.CoreTypes.Point, which currently does not have equality operators overloaded. I plan to submit a separate pull request which fixes this. If that goes through, I suggest using overloaded equality operators.
In addition, it appears mustInteractWithEnvironment is a boolean. If this is the case, you can eliminate the == true (minor code style comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestions, agreed. Will review your PR shortly.
CoolBots
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution! I really like your changes, but I also agree with @glen3b, and therefore I request that you implement his suggestions prior to merge.
Grabbing Glen's changes to upstream. Who uses VIM?
Gravity, Velocity, Position, DrawColor, more for individual points.
Working on a unified PhysicsObject class to treat many points as one object.